BackendFilter/Sort #170
BackendFilter/Sort #170
Conversation
naasanov
left a comment
naasanov
left a comment
There was a problem hiding this comment.
Couple requests involving centralizing logic more and extending to the rest of the modules
| class PaginatedResponse[ModelType](BaseModel): | ||
| """Generic paginated response wrapper matching existing PaginatedResponse.""" | ||
|
|
||
| items: list[ModelType] | ||
| total_records: int | ||
| page_size: int | ||
| page_number: int | ||
| total_pages: int | ||
|
|
||
| @property | ||
| def has_next(self) -> bool: | ||
| """Check if there's a next page.""" | ||
| return self.page_number < self.total_pages | ||
|
|
||
| @property | ||
| def has_prev(self) -> bool: | ||
| """Check if there's a previous page.""" | ||
| return self.page_number > 1 |
There was a problem hiding this comment.
This is now duplicated between core/models.py
|
|
||
| Args: | ||
| page_number: Page number (1-indexed) | ||
| page_size: Items per page (None = all items) | ||
| sort_by: Field to sort by | ||
| sort_order: Sort order ('asc' or 'desc') | ||
| location_id: Filter by location ID | ||
| contact_one_id: Filter by contact one (student) ID | ||
|
|
||
| Returns: | ||
| PaginatedPartiesResponse with items and metadata | ||
| """ |
There was a problem hiding this comment.
This is still a pretty long function. Would be great to have a util function that combines these all together since it will look very similar between all the "get all" routes
| location_id: int | None = None, | ||
| contact_one_id: int | None = None, |
There was a problem hiding this comment.
This is a bit restrictive. These routes will be used to replace the filter in the admin table, which means the function should be able to filter by each column. To acheive this without the function becoming too bulky I'm cool with just extracting the raw dict of query params and then validating them against the allowed fields within the util function
| """Create a party registration from an admin. Both contacts must be specified.""" | ||
| # Get/create location and validate no hold | ||
| location = await self._validate_and_get_location(dto.google_place_id) | ||
|
|
||
| # Get contact_one by email | ||
| contact_one = await self._get_student_by_email(dto.contact_one_email) | ||
|
|
||
| # Create party data with contact_two information directly | ||
| party_data = PartyData( | ||
| party_datetime=dto.party_datetime, | ||
| location_id=location.id, | ||
| contact_one_id=contact_one.account_id, | ||
| contact_two=dto.contact_two, | ||
| ) | ||
|
|
||
| # Create party |
There was a problem hiding this comment.
I prefer having the whitespace here because it makes it more readable, and format changes are outside the scope of the ticket
| # Create a copy to avoid mutating the original | ||
| local_overrides = dict(overrides) | ||
|
|
||
| if "location_id" not in local_overrides: | ||
| location = await self.location_utils.create_one() | ||
| overrides["location_id"] = location.id | ||
| local_overrides["location_id"] = location.id | ||
|
|
||
| if "contact_one_id" not in overrides: | ||
| if "contact_one_id" not in local_overrides: | ||
| student = await self.student_utils.create_one() | ||
| overrides["contact_one_id"] = student.account_id | ||
| local_overrides["contact_one_id"] = student.account_id | ||
|
|
||
| return await super().next_dict(**overrides) | ||
| return await super().next_dict(**local_overrides) |
There was a problem hiding this comment.
This is a good addition. In order to maintain consistency, make these changes in all functions that use overrides across all the util classes
| @pytest.mark.asyncio | ||
| async def test_debug_party_creation(self): | ||
| """Debug test to see what's happening with party creation.""" | ||
| print("\n=== Starting test ===") | ||
|
|
||
| for i in range(5): | ||
| print(f"\n--- Creating party {i + 1} ---") | ||
| party = await self.party_utils.create_one() | ||
| print(f"Created party with ID: {party.id}") | ||
| print(f"Location ID: {party.location_id}, Contact ID: {party.contact_one_id}") | ||
|
|
||
| print("\n=== Checking database ===") | ||
| response = await self.admin_client.get("/api/parties/") | ||
| data = response.json() | ||
| print(f"Total records from API: {data['total_records']}") | ||
| print(f"Number of items: {len(data['items'])}") | ||
| print(f"Item IDs: {[item['id'] for item in data['items']]}") |
There was a problem hiding this comment.
This debug test should be removed for main
| """ | ||
| Tests for party list endpoint with pagination, sorting, and filtering. | ||
|
|
||
| Add these tests to your existing party_router_test.py file or create a new test file. | ||
| """ |
There was a problem hiding this comment.
Since testing pagination and filtering and sorting will result in a lot of bloat across tests, it would be a good idea to make utils that we can reuse to test get all routes across routers. This could be done using parameterized tests, test generators (like in test_templates.py), util functions (like in assertions.py), or mix of all of them.
| parties = result.scalars().all() | ||
| return [party.to_dto() for party in parties] | ||
|
|
||
| async def get_parties_paginated( |
There was a problem hiding this comment.
We need filter/sort/pagination on all "get all" routes. If the logic is properly centralized within the utils function it should just require <=10 lines of configuration for each function
| ) | ||
|
|
||
| # Build query params | ||
| filters: list[FilterParam] = [] |
There was a problem hiding this comment.
Now that we have an overarching filter function, we can make "get parties by (location/contact/date_range/etc.)" delegate to get_parties_paginated
High-level motivation for making the changes
Added server-side pagination, sorting, and filtering to the /api/parties/ endpoint to improve performance and scalability as the database grows. Previously, all filtering and sorting happened client-side, which doesn't scale beyond a few hundred records.
Changes:
Core utilities (backend/src/core/query_utils.py):
Added reusable pagination, sorting, and filtering utilities for SQLAlchemy queries
Implemented PaginationParams, SortParam, FilterParam models with Pydantic validation
Created apply_query_params() to combine filters, sorting, and pagination with security whitelisting
Added get_total_count() to efficiently count filtered results before pagination
Party service (backend/src/modules/party/party_service.py):
Added get_parties_paginated() method with support for:
Optional pagination (page_number, page_size)
Sorting by party_datetime, location_id, contact_one_id, or id
Filtering by location_id and/or contact_one_id
Whitelisted allowed sort/filter fields for security
Party router (backend/src/modules/party/party_router.py):
Updated list_parties endpoint to accept query parameters for pagination, sorting, and filtering
All features are opt-in with sensible defaults (backward compatible)
Tests (backend/test/modules/party/test_party_list_features.py):
Added comprehensive test suite with 22 tests covering:
Pagination edge cases (empty DB, exact pages, remainders, beyond last page)
Sorting (ascending/descending by different fields)
Filtering (single and multiple filters)
Combined features (filter + sort + paginate together)
Bug fixes:
Fixed PartyTestUtils.next_dict() to avoid mutating the overrides dict (was causing test failures)
Fixed get_total_count() to properly count rows instead of columns
Closes #161